eunit: Pass node name to {node, ...} instantiator - #10788

CT Test Results

  2 files   12 suites   3m 17s ⏱️
110 tests 106 ✅ 4 💤 0 ❌
126 runs  122 ✅ 4 💤 0 ❌

Results for commit a1208ab.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

• Complete CT logs (Download Logs)
• HTML Documentation (Download HTML Docs)
• No Windows Installer found

// Erlang/OTP Github Action Bot

Looks good to me. I checked it with the following:

-module(node_test).
-include_lib("eunit/include/eunit.hrl").

node_spawn_test_() ->
    {node, 'foo', fun(Node) ->
        {spawn, Node, [
            {setup, fun remote_setup/0, [
                fun remote/0
            ]}
        ]}
    end}.

remote_setup() ->
    % uncomment to assert that the setup is running on the new node.
    % ?assertEqual(moo, node()),
    ok.

remote() ->
    ?assertEqual(moo, node()),
    ok.

Is automatic enabling of net_kernel safe now? It was disabled on 27 as well.

Is automatic enabling of net_kernel safe now? It was disabled on 27 as well.

Hmm I'm unsure. Where/when was it disabled in OTP 27? I've been trying to locate the specific change but haven't found it. Or do you mean the comments that already exist removed by this PR?

Note the auto-start is intentionally limited to EUnit's {node, ...} test context only and when node() is nonode@nohost, not a global re-enable. If there are specific safety concerns I should address, happy to add docs or a config flag.

Do you recommend removing the auto-start?

Is automatic enabling of net_kernel safe now? It was disabled on 27 as well.

Hmm I'm unsure. Where/when was it disabled in OTP 27? I've been trying to locate the specific change but haven't found it. Or do you mean the comments that already exist removed by this PR?

Note the auto-start is intentionally limited to EUnit's {node, ...} test context only and when node() is nonode@nohost, not a global re-enable. If there are specific safety concerns I should address, happy to add docs or a config flag.

Do you recommend removing the auto-start?

I meant the comment you removed, which is present on maint-27 branch as well. I don't know yet, will have to dig into the code and test if it's ok to do that.
Did you try running it with your change during system initialization? I think the comment is talking about various ways to execute code early as described here: https://www.erlang.org/doc/apps/erts/init.html

Did you try running it with your change during system initialization? I think the comment is talking about various ways to execute code early as described here: https://www.erlang.org/doc/apps/erts/init.html

Yes, I tested it during system initialization using all init flags (-eval, -s, -run, -S) on a non-distributed node. All passed. I've added these as test cases in eunit_SUITE:node_test/1 to ensure this keeps working.

Since this can be considered a bug, could you rebase your branch from patch-base-28 tag, so we can apply it to older branches? We will run this in our CI and see if there are any problems with this automatic start of net_kernel.

could you rebase your branch from patch-base-28 tag

Yep, can do, but I didn't find this tag:

Or do you mean patch-base-27 tag or maint-28 branch?

You're right, it doesn't exist, my bad. You can rebase from tag OTP 28.4

You're right, it doesn't exist, my bad. You can rebase from tag OTP 28.4

Done.